Fix: run the orphaned unit:integrations vitest project from the root test scripts - #236
Fix: run the orphaned unit:integrations vitest project from the root test scripts#236AmaadMartin wants to merge 7 commits into
Conversation
3c40559 to
db1cb0e
Compare
integrations/test/version_test.ts hardcoded `expect(version).toBe('1.3.0')`
while integrations/src/version.ts exports '1.4.0', so the test is currently
red. It went stale unnoticed because the vitest project that owns it
(`unit:integrations`) is invoked by no npm script and no workflow, so the
file has never actually run.
Bumping the literal to '1.4.0' would only re-rot on the next release: the
release automation rewrites integrations/src/version.ts (via the
x-release-please-version annotation) but never touches test literals. Assert
against the version declared in integrations/package.json instead. Both
sides are updated in the same release commit, so the assertion is
self-maintaining, and it guards the invariant actually worth guarding: the
exported constant must not drift from the published package version.
vitest.config.ts declares a `unit:integrations` project owning integrations/test/**/*_test.ts, but the name appeared nowhere else in the repository: no npm script and no workflow invoked it, so those tests never ran. validation.yaml runs `npm run test:coverage` and cross-language-integration.yml runs `npm run test:cross-language`, which between them reached every project except this one. Add `--project unit:integrations` to `test`, `test:unit` and `test:coverage`, positioned after `unit:dev` to match the declaration order in vitest.config.ts. No `test:integrations` script is added on purpose: it would sit one character from the existing `test:integration` (which runs the unrelated `integration` project over tests/integration/) and invite mistakes. `unit:core` and `unit:dev` have no individual scripts either. The coverage thresholds are deliberately left untouched. coverage.include already lists integrations/src/**/*.ts and coverage.all defaults to true, so those files were already in the denominator scored at 0%; running the project only adds to the numerator. Measured over `unit:core + unit:dev` (v8), All files goes 88.94/88.11/89.58/88.94 to 88.95/88.14/89.73/88.95 statements/branches/functions/lines - every metric up.
8136fe9 to
a2c057a
Compare
… projects The two project names differ by one character and by scope: unit:integrations owns the integrations/ workspace package, integration owns the cross-component suite in tests/integration/. That similarity is part of why the former was overlooked by the root test scripts for three releases. Comments only; no project definition, glob, or threshold changes.
|
Independent verification from a duplicate task that was elaborated against the same three defects and is being closed in favour of this PR. Recording the measurements here so they are not lost — all run locally against this branch's exact two-file state ( Postconditions
The only failure in the Mutation proof, both drift directions — the assertion fails when it should:
Anti-rot — bumping both Two things a reviewer might otherwise ask for, which the evidence says not to:
|
The disambiguation between unit:integrations and integration was written twice, once from each side, and each copy restated the include glob two lines below it. Keep the comment on unit:integrations -- the project this change wires in, and the less obvious of the two -- and drop the mirror.
The approved spec scopes this change to the three root test-script strings in package.json and the integrations version test, touching vitest.config.ts "at most" for the coverage thresholds block -- whose expected outcome is no edit at all. The project-naming comment added earlier sits outside that ceiling, so it is removed and the diff is now exactly the two files the fix requires. No behaviour change: the comment never affected project resolution.
integrations/build.js compiles src/index.ts and src/index_web.ts into separate published artifacts -- dist/esm and dist/cjs from the first, the dist/web bundle the package's browser field points at from the second -- so an export added to one entry point and forgotten in the other ships a browser bundle silently missing the symbol. Nothing guarded that, and index_web.ts was the one integrations source file no test reached, sitting at 0% coverage even after the project was wired into the root scripts. Assert the two entry points expose the same export names and the same version binding. The key-set assertion is guarded against passing vacuously if both entry points ever resolve to nothing.
The plan scoped this change to three edits and named index_web.ts coverage as out of scope. The test also asserted the wrong invariant: it required the web entry point to export exactly the node surface, but core/src/index_web.ts re-exports only ./common.js against a 60-line core/src/index.ts, because node-only symbols such as GcsArtifactService and UnsafeLocalCodeExecutor must not reach a browser bundle. The surfaces are meant to diverge, so the first node-only export added to integrations would have failed the test and pushed the wrong fix.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No existing issue. The gap originates in google#449, the PR that added the
integrationspackage together with the
unit:integrationsvitest project.Problem:
vitest.config.tsdeclares aunit:integrationsproject that ownsintegrations/test/**/*_test.ts, but no root script and no workflow ever selected it — the stringunit:integrationsappeared exactly once in the whole repo, in its own declaration. The threeaggregate scripts in
package.jsonenumerate projects by hand and all three omitted it:.github/workflows/validation.yamlruns onlynpm run test:coverage, so that workspace's testshad never executed anywhere — not in CI, and not for a contributor following
CONTRIBUTING.md. This is config drift from google#449, which added the project and theworkspace but did not touch the
--projectenumerations.The dormant suite had already rotted.
integrations/test/version_test.tsassertedexpect(version).toBe('1.3.0')whileintegrations/src/version.tshad moved on to'1.4.0'atthis branch's base — so simply wiring the project in would have turned CI deterministically red.
The two halves have to ship together.
Solution: two edits.
package.json— add--project unit:integrationstotest,test:unitandtest:coverage, positioned afterunit:devso the order mirrorsvitest.config.ts.test:unitis included deliberately: it is a unit project, and leaving it out of the scriptcontributors reach for most would recreate the same drift.
cross-languagestays out of allthree — it has its own workflow (
.github/workflows/cross-language-integration.yml) and needsa Go toolchain.
integrations/test/version_test.ts— assert the exported constant againstintegrations/package.jsoninstead of a literal:Why this and not just retargeting the literal: release-please rewrites
integrations/package.json(release-type: node) andintegrations/src/version.ts(anextra-filesentry) in the same commit, but never touches test files. A retargeted literalwould be stale again on the next release, and the failing check would block the release PR
itself. Anchoring to the manifest pins the invariant that actually matters — the exported
constant equals the published package version — and is self-maintaining. This is not
hypothetical:
mainhas since moved to1.5.0, and the derived assertion holds there with notest edit.
This is a rewrite of an existing test rather than an addition, which the repo guideline
normally discourages. It is the documented exception — the old assertion encoded a value that
was already wrong — and it is in its own commit (
cc69c3e6e), separate from thepackage.jsonchange (a2c057a8b), so the assertion change can be reviewed on its own.How the manifest is read, and why not
readFileSync. The manifest is pulled in with a nativeJSON module import,
import packageJson from '../package.json' with {type: 'json'}, rather thanJSON.parse(readFileSync(new URL('../package.json', import.meta.url), 'utf-8')). The usualargument for the
readFileSyncform is that a JSON import needsresolveJsonModule, which theroot
tsconfig.jsondoes not set — but that argument does not hold in this repo, and it waschecked rather than assumed:
resolveJsonModuleis already on, implied by"module": "nodenext", so notsconfig.jsonchangeis needed and no compiler behaviour changes for
core,devorintegrations.npx tsc --noEmitreports zero errors under
integrations/. The import form is also the established patternhere — 20 existing occurrences across
tests/integration/**— and it yields a typedpackageJson.versiondirectly, where thereadFileSyncform needs a hand-written{version: string}annotation over aJSON.parsethat actually returnsany. Fewer lines, nonew tsconfig surface, better typing, existing convention.
vitest.config.tsis intentionally unchanged. Adding a project can only add covered lines;coverage.includeis untouched so the denominator does not move, andintegrations/src/**wasalready in it at 0% via
coverage.all. Coverage is strictly non-decreasing and the86/87/88/86thresholds are unaffected — CI confirmed every metric rose on every leg (ubuntu90.68/89.61/91.55/90.68 → 90.69/89.62/91.63/90.69). The threshold ratchet is filed separately.
Out of scope, deliberately: a guard preventing this drift class from recurring (queued as its
own task), and coverage for
integrations/src/index_web.ts.Rejected alternatives.
unit:integrationsproject as intentionally dormant. The evidence says oversight,not intent: feat(integrations): create new top-level integrations package google/adk-js#449 wired up every other integration point (workspace entry, project
definition, the alias on all six projects,
coverage.include, the release-please entry) andmissed only the two
scriptslines. It also carries none of the three markers that makecross-language's exclusion deliberate — no dedicated script, no dedicated workflow, notoolchain prerequisite.
@google/adk-integrationsis a published, release-managed package, sodeleting the project just re-arms the same silent-no-run trap for the next contributor.
the next release, and the failing check would land on the release PR itself.
extra-fileswith anx-release-please-versionannotation.That makes the test tautological — automation would write both sides from one value, so it
could never detect a desync — and extends release automation's write surface into test code.
--project '!cross-language').The pinned vitest supports it, and it would structurally prevent this class of drift. Rejected
here because
validation.yamlruns the matrix onubuntu-latest,windows-latestandmacos-latest, and a!inside an npm script argument is a quoting hazard acrossshandcmd.exe. Not worth that risk in the single command that gates every PR, against a four-wordexplicit edit. Worth revisiting on its own.
Review history — one test was added and then removed. An earlier revision of this PR added
integrations/test/index_web_test.ts, asserting that the web entry point exports exactly the nodesurface. It has been removed (
05cfab7f7). It was out of scope, and its stated rationale waswrong for this codebase:
core/src/index_web.tsre-exports only./common.jsagainst a 60-linecore/src/index.ts, because node-only symbols (GcsArtifactService,DatabaseSessionService,UnsafeLocalCodeExecutor) must not reach a browser bundle. The two surfaces are designed todiverge, and there is no test of that kind anywhere in the repo. The first node-only export added
to
integrationswould have failed the test and pressured the wrong fix. Nothing the plan askedfor lost coverage:
index_web.tsreturns to its pre-existing 0%, which the plan explicitlydeclared out of scope.
Collision check:
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000over all 531open PRs, plus
gh pr diff --name-onlyon every adjacent hit. Findings:9f5a6900a..d2db73df0on the test), plus atests/integration/repo_config/vitest_projects_test.tsdrift guard. It was opened 2026-08-02, four days after this PR (2026-07-29), so this one is the
original and Test: fail CI when a vitest project is run by no root package.json script #543 converged on the same implementation independently — which is at least a
useful second data point for the JSON-import form. The guard test in Test: fail CI when a vitest project is run by no root package.json script #543 is the separately
queued follow-up and does not belong here.
main), so this branch is deliberately not rebased: a history rewrite would re-parent orconflict them, and GitHub still reports the merge CLEAN against the advanced
main.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Baseline — the dead project's failure.
npx vitest run --project unit:integrationsbefore thefix, which is the evidence the project was dead:
After the fix:
Mutation proof 1 — the test can actually fail (desync detection). A version test comparing two
values that are equal by construction would carry no signal, so the assertion was proved live by
mutating the source it pins.
integrations/src/version.tstemporarily set to'1.4.1', leavingintegrations/package.jsonat1.4.0:Mutation proof 2 — the test survives a release bump (self-maintenance). This is the property
the old hardcoded literal lacked, so it is proved too. Simulating a release-please commit by
setting both
integrations/src/version.tsandintegrations/package.json'sversionto1.6.0:Both mutations reverted immediately;
git status --porcelainis empty and neitherintegrations/src/version.tsnorintegrations/package.jsonis in the diff (both arerelease-please-owned and must never be hand-edited).
Wiring proof — positive and negative.
npx vitest list --project unit:core --project unit:dev --project unit:integrations(the exact project set of the fixedtest:unit) collects theintegrations/testentries; the same command withmain's two-project selector collects zero.Static gates on the touched files:
npx eslint integrations/test/version_test.tsclean;npx prettier --check integrations/test/version_test.ts package.jsonclean.npx tsc --noEmitreports only pre-existing errors under
core/test/**andtests/**— zero underintegrations/,and the changed test file is confirmed present in the
tscprogram.Coverage-gate run — the check that actually proves this change. It exercises the threshold gate
on the exact projects the fix adds, and is a strict subset of the CI command:
index.tsandversion.tsgo 0% → 100%.index_web.tsstays at 0%: it is a browser-entryre-export nothing imports. That is left alone deliberately — no contrived test for it, and no
coverage-ignore pragma, since the repo guidelines class those as suppressions.
Manual End-to-End (E2E) Tests:
npm install && npm run build. The build is required:tests/global_setup.tsimports@google/adkwith no alias in scope, so it resolves throughcore/dist. Without it everyvitest invocation dies with
Failed to resolve entry for package "@google/adk"andmisleadingly reports
No test files found. This is pre-existing behaviour.npm run test:coverage— the only script CI runs.|unit:integrations|forintegrations/test/version_test.tswith a✓. Onmainno such line exists and the file isabsent from the file list entirely.
git diff main --statlists exactly two files:package.jsonandintegrations/test/version_test.ts.Two local failures on a developer workstation are pre-existing and unrelated, confirmed by
running them on an unmodified checkout:
core/test/code_executors/unsafe_local_code_executor_test.tsanddev/test/cli/cli_create_test.ts(the gcloud-defaults case). Neither file is touched here.The authoritative end-to-end signal is CI on this PR:
validation.yamlrunsnpm run test:coverageon ubuntu, windows and macos, and theunit:integrationsproject nowappears on all three legs.
Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.